Skip to content

Avoid mutating SSLContext in SSLServer.#742

Open
ioquatix wants to merge 1 commit intomasterfrom
set_params-session_id_context
Open

Avoid mutating SSLContext in SSLServer.#742
ioquatix wants to merge 1 commit intomasterfrom
set_params-session_id_context

Conversation

@ioquatix
Copy link
Member

@ioquatix ioquatix commented Apr 21, 2024

I think that we shouldn't mutate the SSLContext in SSLServer and instead be more proactive about setting the right defaults.

               |   FrozenError: can't modify frozen OpenSSL::SSL::SSLContext: #<OpenSSL::SSL::SSLContext:0x0000000104d3f3b0 @verify_mode=0, @verify_hostname=true, @servername_cb=#<Proc:0x0000000104d3f1a8 /Users/samuel/Developer/socketry/async-http-native-io/external/falcon/lib/falcon/environment/proxy.rb:78>, @session_id_context=nil, @max_proto_version=771, @min_proto_version=771>
               |   → /Users/samuel/.gem/ruby/3.3.0/bundler/gems/openssl-d3d857cd1e4d/lib/openssl/ssl.rb:536 in `initialize'
               |     /Users/samuel/.gem/ruby/3.3.0/gems/io-endpoint-0.7.2/lib/io/endpoint/ssl_endpoint.rb:125 in `new'
               |     /Users/samuel/.gem/ruby/3.3.0/gems/io-endpoint-0.7.2/lib/io/endpoint/ssl_endpoint.rb:125 in `block in bind'
               |     /Users/samuel/.gem/ruby/3.3.0/gems/io-endpoint-0.7.2/lib/io/endpoint/ssl_endpoint.rb:124 in `map'
               |     /Users/samuel/.gem/ruby/3.3.0/gems/io-endpoint-0.7.2/lib/io/endpoint/ssl_endpoint.rb:124 in `bind'
               |     /Users/samuel/Developer/socketry/async-http-native-io/lib/async/http/endpoint.rb:185 in `bind'
               |     /Users/samuel/.gem/ruby/3.3.0/gems/io-endpoint-0.7.2/lib/io/endpoint/bound_endpoint.rb:13 in `bound'
               |     /Users/samuel/.gem/ruby/3.3.0/gems/io-endpoint-0.7.2/lib/io/endpoint/bound_endpoint.rb:83 in `bound'
               |     lib/falcon/service/server.rb:41 in `block in start'
               |     /Users/samuel/.gem/ruby/3.3.0/gems/async-2.10.2/lib/async/task.rb:163 in `block in run'
               |     /Users/samuel/.gem/ruby/3.3.0/gems/async-2.10.2/lib/async/task.rb:376 in `block in schedule'

@rhenium
Copy link
Member

rhenium commented Apr 26, 2024

Can we just skip assigning the session ID context if SSLContext is already frozen, for now?

I agree SSLServer shouldn't mutate SSLContext, but I don't feel safe to change the behavior without deprecating it first, as that could break existing user programs in a tricky way.

rhenium
rhenium reviewed Apr 26, 2024
View reviewed changes
lib/openssl/ssl.rb
session_id = prng.bytes(16).unpack1('H*')
self.session_id_context = session_id
end

Copy link
Member

@rhenium rhenium Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SSLContext#set_params is useful only for TLS clients so the session ID context should not be set here (which only makes sense for servers).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rhenium rhenium rhenium left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ioquatix @rhenium